Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Requests: Add "requests.failed" hook #582

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

pprkut
Copy link

@pprkut pprkut commented Oct 26, 2021

Pull Request Type

  • I have checked there is no other PR open for the same change.

This is a:

  • Bug fix
  • New feature
  • Code quality improvement

Context

This allows to inspect or alter the exception that is thrown to
the user in case of transport errors, or in case of response
parsing problems.

Detailed Description

We use the hooks to gather analytics about the remote requests we make in out platform, but failed requests slipped through the cracks since there is currently no way to track those. You can get some analytics from when you make the request, but you won't know from just that whether it failed. We'd also like to know why it failed, the error message that was returned, etc.

We've been using the hook in our platform for a couple weeks now and it worked fine for us :)
(Although we've applied this change to 1.8.x)

Quality assurance

  • This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added unit tests to accompany this PR.
  • The (new/existing) tests cover this PR 100%.
  • I have (manually) tested this code to the best of my abilities.
  • My code follows the style guidelines of this project.

Documentation

For new features:

  • I have added a code example showing how to use this feature to the examples directory.
  • I have added documentation about this feature to the docs directory.
    If the documentation is in a new markdown file, I have added a link to this new file to the Docs folder README.md file.

@jrfnl
Copy link
Member

jrfnl commented Oct 26, 2021

Thank you @pprkut . I'm not adverse to this change, but would like to see it covered by unit tests.

@pprkut
Copy link
Author

pprkut commented Oct 26, 2021

Sure thing, just a bit confused by the test organization 😅
If someone could give me a pointer of where the test(s) should be added, I would much appreciate it.

@jrfnl
Copy link
Member

jrfnl commented Nov 1, 2021

Sure thing, just a bit confused by the test organization 😅 If someone could give me a pointer of where the test(s) should be added, I would much appreciate it.

I think the most appropriate place for the tests would be in the RequestsTest class as the hook is being added to the Requests class.

If needs be, we can always move it later (we're working on improving the test suite).

If you need some inspiration on how to test the hook, you may want to have a look at the following tests:

public function testProgressCallback() {
$mock = $this->getMockBuilder(stdClass::class)->setMethods(array('progress'))->getMock();
$mock->expects($this->atLeastOnce())->method('progress');
$hooks = new Hooks();
$hooks->register('request.progress', array($mock, 'progress'));
$options = array(
'hooks' => $hooks,
);
$options = $this->getOptions($options);
Requests::get(httpbin('/get'), array(), $options);
}
public function testAfterRequestCallback() {
$mock = $this->getMockBuilder(stdClass::class)
->setMethods(array('after_request'))
->getMock();
$mock->expects($this->atLeastOnce())
->method('after_request')
->with(
$this->isType('string'),
$this->logicalAnd($this->isType('array'), $this->logicalNot($this->isEmpty()))
);
$hooks = new Hooks();
$hooks->register('curl.after_request', array($mock, 'after_request'));
$hooks->register('fsockopen.after_request', array($mock, 'after_request'));
$options = array(
'hooks' => $hooks,
);
$options = $this->getOptions($options);
Requests::get(httpbin('/get'), array(), $options);
}

@pprkut
Copy link
Author

pprkut commented Nov 3, 2021

Thank you @jrfnl! I will take a look!

@jrfnl
Copy link
Member

jrfnl commented Nov 26, 2021

@pprkut Just checking in to see how it's going...

@pprkut
Copy link
Author

pprkut commented Nov 26, 2021

@jrfnl I still have it on my list, but haven't found the time yet :-/
It's still gonna take me some time. My goal would be to finish this up before the end of the year

@pprkut
Copy link
Author

pprkut commented Dec 17, 2021

@jrfnl I think I got all the different test cases. Let me know what you think :)

I'm also fine squashing the commits, etc. Just focused on getting the tests in for now

src/Requests.php Outdated Show resolved Hide resolved
src/Requests.php Outdated Show resolved Hide resolved
@m2mobi-mirror m2mobi-mirror force-pushed the failed_hook branch 2 times, most recently from 29202ff to c99d3fc Compare March 15, 2022 14:14
src/Requests.php Outdated Show resolved Hide resolved
@schlessera
Copy link
Member

@pprkut,

Thanks for the work on this PR so far!

I have been thinking about this some more, and I actually fail to see what the use case is where you would need this action. I'm not opposed to adding actions, but they need to add something to warrant their added maintenance cost. Here, I'm not sure this is the case. Maybe you can help me figure this out.

The way I see it right now, you could get everything you need by adding a try/catch block around the Requests::request() call, instead of doing the call right away and then listening on a requests/failed hook for a possible exception.

Do you have a use case in mind where just adding a try/catch block around Requests::request() would not be enough or not be feasible?

@pprkut
Copy link
Author

pprkut commented Apr 25, 2022

@schlessera Sure, I'll try to outline our usecase :)

We use the hooks to collect analytics about all HTTP requests that our system makes. The end result then looks like this:

Screenshot_20220425_110644

The hooks we use are:

  • requests.before_request
  • curl.after_request
  • requests.before_redirect
  • requests.after_request

The way it works is that requests.before_request initiates the analytics data, but we need the response data to finalize it. curl.after_request adds IP information to the analytics, requests.before_redirect finalizes a request with a 301 response (for example), and requests.after_request finalizes all other successful requests.

However, if a request fails, because of wrong options or curl errors, we have the analytics data initiated, but no other hook is triggered so we never get an indicator that the request is "completed" (and that means the anayltics are never stored and won't show on that dashboard). We could do that with exception handling on our side of the code, but then we'd have to either do that everywhere we make an HTTP request with Requests and call the analytics library in all those places to indicate the request is completed and failed, or implement a wrapper around Requests that does that exception handling. But that just seems weird and clunky since most of the functionality is already handled via hooks.

Adding the requests.failed hook looked like the cleanest way to implement this, and would also allow other people to do something similar. That's why I went in that direction.

@SMillerDev
Copy link

Rebased, and I think the "Status: needs tests" label can go now since it has tests.

@SMillerDev
Copy link

Rebased after doing some work on our fork accidentally dropped some commits.

@jrfnl
Copy link
Member

jrfnl commented Feb 12, 2024

@pprkut PR #833 has just been merged, which should allow for simplifying the tests in this PR. Would you like to update the PR ? Or do you expect us to get your PR to a mergable state ?

@pprkut
Copy link
Author

pprkut commented Feb 12, 2024

@jrfnl I've been following along the other PR 🙂
I'm on vacation right now until the weekend, but I'll see what I can do to get this PR updated 👍

@jrfnl
Copy link
Member

jrfnl commented Feb 12, 2024

@pprkut Excellent! Take your time and I look forward to seeing the next iteration. In the mean time: enjoy your break!

@pprkut
Copy link
Author

pprkut commented Feb 28, 2024

@jrfnl Should be updated 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants